-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Remove the #[no_sanitize]
attribute in favor of #[sanitize(xyz = "on|off")]
#142681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_passes/src/check_attr.rs The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. cc @BoxyUwU, @jieyouxu, @Kobzol
Some changes occurred in src/doc/unstable-book/src/language-features/no-sanitize.md cc @rust-lang/project-exploit-mitigations, @rcvalle Some changes occurred in tests/codegen/sanitizer cc @rcvalle Some changes occurred in tests/ui/sanitizer cc @rcvalle Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs |
Thank you very much for your time and work on this, @1c3t3a! Much appreciated. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
It doesn't matter for me, but should we make this change gradually by supporting both for some time? |
That's possible for sure, I put stuff into different commits for that reason and I can just drop the middle commit to have both exist in parallel. What's the reason for having both at the same time? An easier migration period? |
We generally do not make such efforts to migrate usage of nightly features without a specific known good reason to do so. (Let us know of course if there is one, e.g. if this affects RfL in some way.) |
☔ The latest upstream changes (presumably #142770) made this pull request unmergeable. Please resolve the merge conflicts. |
We don't use |
I rebased the change to make review easier. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if this is a built-in attribute, then IIUC #[sanitize]
(even if unstably gated) could introduce new name resolution ambiguities with user macros or derive macro attributes and thus technically break existing user code. See for instance #143834. cf. #134963.
// tests/ui/whatever_subdir/test.rs
//@ proc-macro: my_derive.rs
//@ edition: 2024
use my_derive::MyDerive;
#[derive(MyDerive)]
#[sanitize]
//~^ ERROR `sanitize` is ambiguous
pub struct Foo;
fn main() {}
// tests/ui/whatever_subdir/auxiliary/my_derive.rs
//@ edition: 2024
extern crate proc_macro;
use proc_macro::{Span, TokenStream};
#[proc_macro_derive(
MyDerive,
attributes(
sanitize
)
)]
pub fn derive_custom(_item: TokenStream) -> TokenStream {
TokenStream::new()
}
You can still observe this for #[no_sanitize]
too, if you switch sanitize
for no_sanitize
in the above test case.
error: malformed `no_sanitize` attribute input
--> /home/joe/repos/rust/tests/ui/bitbuffer_derive/test.rs:7:1
|
LL | #[no_sanitize]
| ^^^^^^^^^^^^^^ help: must be of the form: `#[no_sanitize(address, kcfi, memory, thread)]`
error[E0659]: `no_sanitize` is ambiguous
--> /home/joe/repos/rust/tests/ui/bitbuffer_derive/test.rs:7:3
|
LL | #[no_sanitize]
| ^^^^^^^^^^^ ambiguous name
|
= note: ambiguous because of a name conflict with a builtin attribute
= note: `no_sanitize` could refer to a built-in attribute
error[E0658]: the `#[no_sanitize]` attribute is an experimental feature
--> /home/joe/repos/rust/tests/ui/bitbuffer_derive/test.rs:7:1
|
LL | #[no_sanitize]
| ^^^^^^^^^^^^^^
|
= note: see issue #39699 <https://github.com/rust-lang/rust/issues/39699> for more information
= help: add `#![feature(no_sanitize)]` to the crate attributes to enable
= note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date
error: aborting due to 3 previous errors
Some errors have detailed explanations: E0658, E0659.
For more information about an error, try `rustc --explain E0658`.
Or just
macro_rules! sanitize {
() => {
/* .. */
};
}
pub(crate) use sanitize; // `use` here becomes ambiguous
fn main() {}
I imagine no_sanitize
is much less likely to collide with something a user would write (not an argument against this change, just an observation re. technically a breaking change).
I don't know how much of a concern for breakage this is in practice, a crater run may or may not be a good idea. If the breakage is deemed acceptable, then this should get relnotes.
☔ The latest upstream changes (presumably #143919) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
@rustbot labels +T-lang +needs-fcp Given that this is breaking, we should review and FCP. Let's do that crater run, since we'll need it for our review. @bors2 try |
@bors r- |
This change implements the #[sanitize(..)] attribute, which opts to replace the currently unstable #[no_sanitize]. Essentially the new attribute works similar as #[no_sanitize], just with more flexible options regarding where it is applied. E.g. it is possible to turn a certain sanitizer either on or off: `#[sanitize(address = "on|off")]` This attribute now also applies to more places, e.g. it is possible to turn off a sanitizer for an entire module or impl block: ```rust \#[sanitize(address = "off")] mod foo { fn unsanitized(..) {} #[sanitize(address = "on")] fn sanitized(..) {} } \#[sanitize(thread = "off")] impl MyTrait for () { ... } ``` This attribute is enabled behind the unstable `sanitize` feature.
This removes the #[no_sanitize] attribute, which was behind an unstable feature named no_sanitize. Instead, we introduce the sanitize attribute which is more powerful and allows to be extended in the future (instead of just focusing on turning sanitizers off). This also makes sanitize(kernel_address = ..) attribute work with -Zsanitize=address To do it the same as how clang disables address sanitizer, we now disable ASAN on sanitize(kernel_address = "off") and KASAN on sanitize(address = "off"). The same was added to clang in https://reviews.llvm.org/D44981.
90c564e
to
95bdb34
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Argh, I rebased it now! @jieyouxu: Was your r- only related to the merge conflicts are also to other parts? |
Only for merge conflict, it was at the time still in the queue. |
Alright, happy to hear! Should we give it another try then? Before I get the next merge conflict :D |
@rustbot ready |
Alright. Let's try it again. |
@bors r+ |
Remove the `#[no_sanitize]` attribute in favor of `#[sanitize(xyz = "on|off")]` This came up during the sanitizer stabilization (rust-lang#123617). Instead of a `#[no_sanitize(xyz)]` attribute, we would like to have a `#[sanitize(xyz = "on|off")]` attribute, which is more powerful and allows to be extended in the future (instead of just focusing on turning sanitizers off). The implementation is done according to what was [discussed on Zulip](https://rust-lang.zulipchat.com/#narrow/channel/343119-project-exploit-mitigations/topic/Stabilize.20the.20.60no_sanitize.60.20attribute/with/495377292)). The new attribute also works on modules, traits and impl items and thus enables usage as the following: ```rust #[sanitize(address = "off")] mod foo { fn unsanitized(..) {} #[sanitize(address = "on")] fn sanitized(..) {} } trait MyTrait { #[sanitize(address = "off")] fn unsanitized_default(..) {} } #[sanitize(thread = "off")] impl MyTrait for () { ... } ``` r? `@rcvalle`
Remove the `#[no_sanitize]` attribute in favor of `#[sanitize(xyz = "on|off")]` This came up during the sanitizer stabilization (rust-lang#123617). Instead of a `#[no_sanitize(xyz)]` attribute, we would like to have a `#[sanitize(xyz = "on|off")]` attribute, which is more powerful and allows to be extended in the future (instead of just focusing on turning sanitizers off). The implementation is done according to what was [discussed on Zulip](https://rust-lang.zulipchat.com/#narrow/channel/343119-project-exploit-mitigations/topic/Stabilize.20the.20.60no_sanitize.60.20attribute/with/495377292)). The new attribute also works on modules, traits and impl items and thus enables usage as the following: ```rust #[sanitize(address = "off")] mod foo { fn unsanitized(..) {} #[sanitize(address = "on")] fn sanitized(..) {} } trait MyTrait { #[sanitize(address = "off")] fn unsanitized_default(..) {} } #[sanitize(thread = "off")] impl MyTrait for () { ... } ``` r? ``@rcvalle``
Rollup of 19 pull requests Successful merges: - #140956 (`impl PartialEq<{str,String}> for {Path,PathBuf}`) - #141744 (Stabilize `ip_from`) - #142681 (Remove the `#[no_sanitize]` attribute in favor of `#[sanitize(xyz = "on|off")]`) - #142871 (Trivial improve doc for transpose ) - #144252 (Do not copy .rmeta files into the sysroot of the build compiler during check of rustc/std) - #144476 (rustdoc-search: search backend with partitioned suffix tree) - #144567 (Fix RISC-V Test Failures in ./x test for Multiple Codegen Cases) - #144804 (Don't warn on never to any `as` casts as unreachable) - #144960 ([RTE-513] Ignore sleep_until test on SGX) - #145013 (overhaul `&mut` suggestions in borrowck errors) - #145041 (rework GAT borrowck limitation error) - #145243 (take attr style into account in diagnostics) - #145405 (cleanup: use run_in_tmpdir in run-make/rustdoc-scrape-examples-paths) - #145432 (cg_llvm: Small cleanups to `owned_target_machine`) - #145484 (Remove `LlvmArchiveBuilder` and supporting code/bindings) - #145557 (Fix uplifting in `Assemble` step) - #145563 (Remove the `From` derive macro from prelude) - #145565 (Improve context of bootstrap errors in CI) - #145584 (interpret: avoid forcing all integer newtypes into memory during clear_provenance) Failed merges: - #145359 (Fix bug where `rustdoc-js` tester would not pick the right `search.js` file if there is more than one) - #145573 (Add an experimental unsafe(force_target_feature) attribute.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #142681 - 1c3t3a:sanitize-off-on, r=rcvalle Remove the `#[no_sanitize]` attribute in favor of `#[sanitize(xyz = "on|off")]` This came up during the sanitizer stabilization (#123617). Instead of a `#[no_sanitize(xyz)]` attribute, we would like to have a `#[sanitize(xyz = "on|off")]` attribute, which is more powerful and allows to be extended in the future (instead of just focusing on turning sanitizers off). The implementation is done according to what was [discussed on Zulip](https://rust-lang.zulipchat.com/#narrow/channel/343119-project-exploit-mitigations/topic/Stabilize.20the.20.60no_sanitize.60.20attribute/with/495377292)). The new attribute also works on modules, traits and impl items and thus enables usage as the following: ```rust #[sanitize(address = "off")] mod foo { fn unsanitized(..) {} #[sanitize(address = "on")] fn sanitized(..) {} } trait MyTrait { #[sanitize(address = "off")] fn unsanitized_default(..) {} } #[sanitize(thread = "off")] impl MyTrait for () { ... } ``` r? ```@rcvalle```
Bors hasn't noticed that this was merged. @bors r- retry |
This came up during the sanitizer stabilization (#123617). Instead of a
#[no_sanitize(xyz)]
attribute, we would like to have a#[sanitize(xyz = "on|off")]
attribute, which is more powerful and allows to be extended in the future (insteadof just focusing on turning sanitizers off). The implementation is done according to what was discussed on Zulip).
The new attribute also works on modules, traits and impl items and thus enables usage as the following:
r? @rcvalle